-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Provide Edit support in Sources tab for multi-source app (#17588) #17890
feat: Provide Edit support in Sources tab for multi-source app (#17588) #17890
Conversation
360a3b0
to
1b9ff88
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@keithchong , tried testing the PR out. Please find couple of my findings below:
-
The Sources page allows to edit multiple sources at the same time. But, saving one source successfully leads to reload of the entire page which leads to loosing unsaved edits in another source. I think, we should not allow users to edit multiple apps at the same time or add a note somewhere that editing individual source will update only that specific source on the application.
-
The ref field is missing from the expanded source panel.
- We allow users to edit both upper and lower section of the source. But the lower section does not get updated as soon as we change the source type from upper section to a different type (e.g. Directory to Helm or vice versa). Maybe make the other section non-editable while update one?
1b9ff88
to
c699673
Compare
Hi @ishitasequeira , thanks for testing this out. For point 1, yeah, I only disable the lower or upper Edit button within the one source card. I don't disable all the other source cards' Edit buttons. This is something I will have to address later as a 'fit and finish' issue, because the main path is there. For point 2, done. Merge issue from stage 1 PR. I also added a span element so it will always show up even if there is no value there. (See ref in the video below) For point 3, I thought this was working before. Is the following video what you're describing? I also included a fix for another weird issue where for single source apps only, the Helm type parameters updated fine when changed, but when you save the changes, there were duplicate entries for Values and Value Files n the view. |
@ishitasequeira , I had the mechanism set up so that within one source card, the Edit button is disabled if the other is clicked. The change might have been in my stage 3 branch for Add and Delete, and I didn't move it over for this PR. I've just added the fix here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@keithchong it seems the ref
field was added but it does not reflect the value when in edit mode. Added a recording of the same.
Screen.Recording.2024-04-26.at.4.51.05.PM.mov
Hey @keithchong , thanks for fixing this. I found another thing. For the source with the |
content: ( | ||
<DataLoader key='appDetails' input={application} load={app => getSources(app)}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not very much familiar with dataloader but don't we need DataLoader? how you are passing the app
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ashutosh16 , thanks for reviewing this. In my first stage change (for showing all the sources in read-only mode, with the Edit button disabled), this DataLoader is used to load all the sources all at once. See the call to getSources function, which I've now removed below. It calls the appDetails api to get all the details of the sources. If I left it this way, that means that once you visit the Sources (formerly the Parameters) tab, all the data is loaded all at once for all the collapsible sections, regardless if they are collapsed or not. Similarly, even before any of my changes, for the single source case, it loads once you visit the tab.
With this new approach, I delayed the loading for each source until the collapsible section is expanded. This improves performance and avoids flickering when any change in the sources occur. The DataLoader code is moved to application-parameters.tsx. For multiple sources, see line 250. For the single source, see line 192. This explains my second bullet point here: #17890 (comment)
The Loading message appears within each source card, and not outside all the cards.
Note that I was not able to reduce all the flickering/refreshing that is happening, when the underlying repo details is being changed, and the components need to be updated, but it appears to be acceptable and I think it is the same as if the single source is being updated too.
Note that I refactored it so that when the single source field is 'deprecated' or 'removed', we could more easily remove the code related to how the single source is handled (even if there is a bit of duplication). Also, I tried to minimize any potential regressions against the original single source behavior.
Hi Ishita, your concern applies to a single source application too. I'm not changing the values of any input parameter, or adding any new parameters except for the ref field. If the values are incorrect, then we will need a new issue for this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@keithchong Overall PR looks great. Could you resolve the merge conflicts?
663e462
to
cf72c8a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
…roj#17588) Signed-off-by: Keith Chong <kykchong@redhat.com>
cf72c8a
to
6385f7a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested the changes. Overall code looks good. Thanks @keithchong!!
…roj#17588) (argoproj#17890) Signed-off-by: Keith Chong <kykchong@redhat.com>
…roj#17588) (argoproj#17890) Signed-off-by: Keith Chong <kykchong@redhat.com>
…roj#17588) (argoproj#17890) Signed-off-by: Keith Chong <kykchong@redhat.com> Signed-off-by: Javier Solana <javier.solana@cabify.com> Signed-off-by: Javier Solana <javier.solana@cabify.com>
Fixes #17588
This PR builds upon #17275
This PR adds/enables the "Edit" button to the Source and Parameter sections of each collapsible panel under the new Sources tab added from #17275.
There are a few noteworthy design points to this:
Testing:
Checklist: